feat(auto-increment): add transactional epoch fence#26027
Conversation
Co-authored-by: ULookup <CHBulookup@outlook.com>
…uto-increment-epoch-fence
Co-authored-by: ULookup <CHBulookup@outlook.com>
Co-authored-by: ULookup <CHBulookup@outlook.com>
…uto-increment-epoch-fence
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Approved at exact head a7510bf.
Correctness review:
- The dedicated epoch is persisted independently of ordinary schema version and TN enforces monotonic current+1 transitions with overflow rejection.
- CN propagation is closed across in-memory writes, object writes, deletes, soft deletes, transfer, workspace flush, and CN-object compaction. Flush grouping includes table+epoch+known, so generations cannot be coalesced accidentally.
- The live race is closed by TAE prepare ordering: DML prepared first publishes its prepare watermark before a later ALTER validates; ALTER prepared first publishes a committing schema node that later DML waits on and then rejects by epoch.
- Legacy unknown writers map to epoch zero only, so they remain compatible before the first reset and fail closed afterward.
- WAL replay obtains the real dirty-table set from the serialized TxnMemo; replayed prepared transactions fence the table until both commit and rollback terminal paths resolve them.
Unhappy-path review found no blocking leak, hang, stale-fence, or unbounded-growth path. The committed watermark is monotonic; prepared replay registrations have symmetric commit/rollback cleanup; schema waits terminate through the transaction state transition.
Fresh local validation at this exact head passed both normal and race-focused matrices for disttae, catalog, rpc, txnimpl, and TAE db tests. git diff --check also passed. Current GitHub CI is green.
Non-blocking requirements for the follow-up user-facing integration:
- The allocator reset/cache protocol must guarantee that the epoch attached to a write is the generation that actually allocated its values; merely reading a refreshed relation epoch after allocation would be insufficient.
- Enabling reset must be gated against old TNs that do not enforce these new protobuf fields.
- COPY/CLONE reconciliation and end-to-end restart/multi-CN coverage should remain explicit acceptance gates, as stated in #23143.
Process nit: the PR body still says intentionally Draft, while the PR is currently Ready for review.
Merge Queue Status
This pull request spent 22 minutes 41 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonPull request #26027 has been dequeued Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Tick the box to put this pull request back in the merge queue (same as
|
## What type of PR is this? - [x] BUG - [x] Test and CI ## What this PR does / why we need it This PR fixes two independent nondeterministic test setups without relaxing production behavior. ### TAE checkpoint replay TestAppendAndGC2 created its database and tables through low-level TAE catalog APIs. Those APIs mutate the in-memory catalog but do not append rows to mo_database, mo_tables, and mo_columns. After a forced checkpoint truncates the DDL WAL, checkpoint replay can therefore lose the still-live database. The prepared-DML replay fence added by #26027 exposed this invalid setup as ExpectedEOB. The previous version of this PR skipped missing catalog entries, but diagnostics proved the missing IDs were the live test database and table. That production-code relaxation was removed. The test now uses the full DDL helpers and, after restart, opens both relations and scans every column to verify that each still contains all 99 rows. ### Shard read compatibility test TestReadValidatesAllRemoteShardsBeforeSending could create its table before all three CNs reported to the shard server. In the failed Ubuntu job, all shards were initially allocated to cn1 and balancing the second shard took 60 seconds. The shared setup context expires after 10 seconds, so the first remote compatibility check returned context deadline exceeded after one Adjust call instead of reaching the intended incompatible CN. The test now waits for every CN before creating shards, bounds every replica wait, uses separate setup and read contexts, and asserts the exact incompatibility error. This keeps the test fast and prevents unrelated timeout errors from satisfying require.Error. No production code is changed. ## Validation - TAE focused replay test: count=20 - TAE focused replay test: race count=10 - pkg/vm/engine/tae/db/test full package - pkg/vm/engine/tae/txn/txnimpl full package - shard compatibility test: count=30 - shard compatibility test: race count=10 - pkg/shardservice full package - go build ./pkg/shardservice - go vet ./pkg/shardservice - git diff --check
This is the next prerequisite after #26027. It does not expose `ALTER TABLE ... AUTO_INCREMENT=N` yet. The invariant in this PR is that an AUTO_INCREMENT offset reset and its allocator-cache transition belong to the same transaction: - the resetting transaction sees its pending offset without publishing it to other transactions; - statement or transaction rollback retires the private reset cache; - commit publishes a cache for the allocation epoch actually used by the write; - stale builders and old cache generations cannot become active after reset/reload/close; - accepted allocator actions drain during close, while a canceled queued reset cannot mutate the store. The shared service interface now carries `autoIncrEpoch` and `TxnOperator` through both production allocation callers. `memStore` and `sqlStore` gain monotonic `SetOffset` plus DDL-constrained `ForceSetOffset`; all implementations and mocks are updated atomically. Out of scope for this PR: - planner/compiler support for the user-facing ALTER syntax; - COPY/CLONE allocator reconciliation; - rolling-upgrade enablement for mixed-version TNs; - end-to-end BVT for the final SQL behavior. Diff size, separated for reviewability: - production: `+1303 / -86` - tests: `+1832 / -8` - mocks: `+292 / -18` Validation: - `go test -race ./pkg/incrservice -count=1` - `go test ./pkg/sql/colexec/preinsert ./pkg/sql/colexec/table_clone ./pkg/frontend/test ./pkg/vm/engine/test/testutil -count=1` - `go test ./pkg/... -run '^$'` - `git diff --check` - exact-head `mo-pr-preflight-review`: PASS at `891781d3a927b80b54d38e381ebee0d749c1c589` Approved by: @XuPeng-SH, @gouhongshen
What type of PR is this?
Which issue(s) this PR fixes:
issue #23143
What this PR does / why we need it:
This is the first AUTO_INCREMENT-specific prerequisite split from the closed #25883. It does not expose
ALTER TABLE ... AUTO_INCREMENT=Nyet.The invariant introduced here is: a CN write carries the AUTO_INCREMENT epoch observed when its values were allocated, and TN accepts the write only when that epoch matches the table epoch. Once a table enters a fenced epoch, legacy writers that cannot provide an epoch fail closed.
SchemaExtra/plan.TableDef, propagate it through disttae workspace, row and object writes, flush/compaction, RPC, and TN relationscurrent + 1Additional guards reject epoch exhaustion before mutation and keep ordinary schema ALTER operations from advancing the AUTO_INCREMENT epoch.
Explicit non-goals
ALTER TABLE ... AUTO_INCREMENT=Nplanner or compile entry point;Diff size
+1283/-131;+1565/-1079;Validation
go test ./pkg/vm/engine/disttae ./pkg/vm/engine/tae/catalog ./pkg/vm/engine/tae/rpc ./pkg/vm/engine/tae/txn/txnimpl ./pkg/vm/engine/tae/db/test-racecoverage for epoch transitions and replay commit/rollback lifecyclegit diff --checkmo-pr-preflight-review: PASS at exact head4122a7add078c79757121a4add2d8771113c7980mo-self-review: no open blockerThe PR is intentionally Draft while its full CI runs and while this prerequisite is reviewed independently.